-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix triple click issue in paragraph with widget #15011
Fix triple click issue in paragraph with widget #15011
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above comment is nitpicky and optional. I'd like to see it included, but I'm fine with PR as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( domEventData.domEvent.detail >= 3 ) { | ||
setSelectionInAncestorElement(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should narrow it to just affected cases with widgets? This looks like first we check if in nested editable on FF or Safari and then we do it everywhere.
// If triple click should select entire paragraph. | ||
if ( parentElement && | ||
parentElement.is( 'element' ) && | ||
!isWidget( parentElement ) && | ||
domEventData.domEvent.detail >= 3 | ||
) { | ||
this._setSelectionInAncestorElement( element, domEventData ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are checking the parent element? I'm not sure what this condition is checking.
/** | ||
* Selects entire block content, e.g. on triple click it selects entire paragraph. | ||
*/ | ||
private _selectBlockContent( element: ViewElement ): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code could be simpler, we could use some existing helper methods. Let's talk about it f2f.
Suggested merge commit message (convention)
Fix (widget): Triple click on paragraph with widget should select the whole paragraph. Closes #11130.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.